Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vm: fix accountexists bug in pre-SD hardforks #1516

Merged
merged 8 commits into from
Oct 6, 2021

Conversation

acolytec3
Copy link
Contributor

@acolytec3 acolytec3 commented Oct 5, 2021

Fixes #1502 based on test case and suggestion from @jochem-brouwer. I've added the test case to the VM tests though not sure if we want to restructure it along the lines of ethereum-tests somehow. Seems like there ought to be a good way to do that and I can explore how to open that PR if we like this approach.

I have confirmed that this resolves the specific issue when syncing the client on mainnet and it is able to progress beyond this block but I've encountered another block 1233073 with an invalid ReceiptsTrie so will need some additional review to see if my code change caused this new issue or if it's something else now.

@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #1516 (25def52) into master (a50fb5f) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
block 86.48% <ø> (ø)
blockchain 82.78% <ø> (ø)
client 82.64% <ø> (-0.49%) ⬇️
common 94.08% <ø> (-0.19%) ⬇️
devp2p 82.66% <ø> (ø)
ethash 90.76% <ø> (ø)
tx 87.47% <ø> (ø)
vm 79.51% <100.00%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Co-authored-by: Jochem Brouwer <jochembrouwer96@gmail.com>
@ryanio
Copy link
Contributor

ryanio commented Oct 6, 2021

smallest nit ever, but let's move the test file to the appropriate dir and rename to camelCase: packages/vm/test/api/state/homestead-accountExists.spec.ts

otherwise, looks really nice :)

@acolytec3
Copy link
Contributor Author

smallest nit ever, but let's move the test file to the appropriate dir and rename to camelCase: packages/vm/test/api/state/homestead-accountExists.spec.ts

otherwise, looks really nice :)

Done!

jochem-brouwer
jochem-brouwer previously approved these changes Oct 6, 2021
Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great additions!

@acolytec3 acolytec3 merged commit fb4c15e into master Oct 6, 2021
@acolytec3
Copy link
Contributor Author

Just a note, this is causing the follow EthereumTests to fail:

--fork=Homestead:
    CREATE_EmptyContractAndCallIt_0wei
    CREATE_EmptyContractAndCallIt_1wei
    CREATE_EmptyContractWithStorageAndCallIt_0wei
    CREATE_EmptyContractWithStorageAndCallIt_1wei

--fork=TangerineWhistle:
    CREATE_EmptyContractAndCallIt_0wei
    CREATE_EmptyContractAndCallIt_1wei
    CREATE_EmptyContractWithStorageAndCallIt_0wei
    CREATE_EmptyContractWithStorageAndCallIt_1wei
    tx_e1c174e2 

Run npm run tester -- --state --fork=TangerineWhistle --test='CREATE_EmptyContractAndCallIt_1wei' to verify.

I believe the issue is related to the CREATE opcode in the tests where a child contract is being created and we're incorrectly reporting the created account as not existing in the stateManager.accountExists method. More research and a PR to hopefully follow.

@jochem-brouwer
Copy link
Member

Oops that is not good. We should have ran all tests in the ci

@acolytec3
Copy link
Contributor Author

acolytec3 commented Oct 7, 2021 via email

@jochem-brouwer
Copy link
Member

I don't think we should run all hardforks in VM PRs, but we should if it clearly hits a hardfork which is not covered by tests (mostly rather old forks).

@acolytec3
Copy link
Contributor Author

I've opened a new issue to address the issue.

@holgerd77 holgerd77 deleted the preSDaccountexistencebug branch October 12, 2021 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RunBlock calculates incorrect gas usage for contract interaction
3 participants